Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes an XSS vulnerability in the AnnouncementModal component by introducing DOMPurify to sanitize HTML content fetched from a remote announcement API before it is rendered via dangerouslySetInnerHTML.
Changes:
- Adds DOMPurify as a runtime dependency and configures a strict allowlist (tags, attributes, and URI schemes) for HTML sanitization.
- Sanitizes the announcement content before hashing and displaying it, and skips rendering if the sanitized result is empty.
- Updates
package-lock.jsonto reflect the new dependency tree.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/components/AnnouncementModal.tsx |
Adds sanitizeAnnouncementHtml using DOMPurify with a strict allowlist; now sanitizes content before hashing and setting state |
package.json |
Adds dompurify (runtime) and @types/dompurify (redundant stub) as dependencies |
package-lock.json |
Lockfile updated to include dompurify and the redundant @types/dompurify |
Issues found:
-
@types/dompurifyis a deprecated no-op (package.jsonline 15): Thepackage-lock.jsonitself flags it as deprecated: "This is a stub types definition. dompurify provides its own type definitions, so you do not need this installed." It should be removed frompackage.json. -
Reverse tabnabbing via
targetattribute (AnnouncementModal.tsxline 16):targetis included inANNOUNCEMENT_ALLOWED_ATTR, meaning announcement authors (or a compromised API) can injecttarget="_blank"on links. Withoutrel="noopener noreferrer", the opened page can accesswindow.openerand redirect the parent tab. Either removetargetfrom the allowlist, or add a DOMPurifyafterSanitizeAttributeshook to enforcerel="noopener noreferrer"on anytarget="_blank"link. -
One-time re-display for existing users (
AnnouncementModal.tsxline 53, nit): The hash is now computed on sanitized content rather than raw content. Existing users who stored a hash of the raw announcement will see the current announcement one more time. This is a minor, one-time UX regression to be aware of.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
补充一下这次修复后的测试结果（针对
测试过程中首次跑出了一个边界问题： 结果：High-intensity sanitizer test passed（Deterministic=6, Fuzz=12000, Elapsed=112928ms），且 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@codex Please confirm if this feature is necessary and whether it could impact normal announcement behavior (e.g., announcements often contain buttons). |
|
Codex couldn't complete this request. Try again later. |
|
@codex Please confirm if this feature is necessary and whether it could impact normal announcement behavior (e.g., announcements often contain buttons). |
|
Codex couldn't complete this request. Try again later. |
xzdmycbx
left a comment
xzdmycbx
left a comment
There was a problem hiding this comment.
Right now the announcement sanitizer strips style (FORBID_ATTR includes style, and it’s not in ALLOWED_ATTR). This breaks our existing announcement content that relies on inline styles to render “button-like” links (layout/spacing + CTA button appearance). We also have existing announcements with CTA/button jump behavior, and the current sanitization removes the necessary pieces so the CTAs degrade into plain text/unstyled links.
Please update the implementation to:
- Allow
styleattributes to pass through for announcement HTML (at least for a controlled set of tags likea,div,span,p, etc.), so our CTA buttons and layout are preserved. - Ensure the announcement “button jump/redirect” behavior works as intended for existing content (e.g., the CTA links should still behave like buttons and remain usable after sanitization).
Important: keep the security guarantees. If you’re concerned about enabling arbitrary inline CSS, we can restrict allowed CSS properties (e.g., via DOMPurify hooks / a CSS allowlist) rather than forbidding style entirely. But as-is, this is a functional regression for announcements that include CTA buttons.
Summary
DOMPurify对公告 HTML 做白名单净化，修复dangerouslySetInnerHTML场景下的 XSS 风险script/iframe/style/svg/math等高风险标签与style属性_blank的target会被移除_blank链接会强制带上rel="noopener noreferrer"https、mailto、tel、以及站内相对路径（并拒绝//协议相对 URL）Review Feedback Addressed
@types/dompurifyhttp://与协议相对 URL 偏离安全意图Test Plan
npm run build通过target=""残留问题后复测通过